-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Implement unstable trait impl #140399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Implement unstable trait impl #140399
Conversation
d01f12a
to
9e139e1
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
486d3d3
to
02f780f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
567ec89
to
80fd239
Compare
☔ The latest upstream changes (presumably #142299) made this pull request unmergeable. Please resolve the merge conflicts. |
850e3e1
to
27d9e21
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls add PR description providing an outline of the approach
apart from that, a few minor nits, after this r=me :3
Thank you for working on this ❤️
// enabled that we do not. | ||
// | ||
// Note: we don't consider a feature to be enabled | ||
// if we are in std/core even if there is a corresponding `feature` attribute on the crate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
future work: we likely want an #[rustc_allow_internal_unstable(foo)]
attribute which only adds UnstableFeature
clauses to the param_env, but not predicates_of
.
tests/ui/unstable-feature_bound/unstable-feature-cross-crate-multiple-symbol.rs
Outdated
Show resolved
Hide resolved
tests/ui/unstable-feature_bound/unstable-feature-cross-crate-require-bound.rs
Show resolved
Hide resolved
tests/ui/unstable-feature_bound/auxiliary/unstable_impl_codegen_aux2.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: lcnr <[email protected]>
compiler/rustc_trait_selection/src/error_reporting/traits/ambiguity.rs
Outdated
Show resolved
Hide resolved
|
||
let mut err = self.dcx().struct_span_err( | ||
span, | ||
format!("unstable feature `{sym}` is used without being enabled."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would try to mirror the existing errors for unstable library features if staged_api
is not enabled
error[E0658]: use of unstable library feature `slice_partition_dedup`
--> src/main.rs:3:7
|
3 | x.partition_dedup();
| ^^^^^^^^^^^^^^^
|
= note: see issue #54279 <https://github.com/rust-lang/rust/issues/54279> for more information
= help: add `#![feature(slice_partition_dedup)]` to the crate attributes to enable
= note: this compiler was built on 2025-07-02; consider upgrading it if it is out of date
not sure whehter you could even reuse the reporting machinery we already use for methods etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diagnostics can be reported through feature_err_issue
, but it requires the issue number of the library feature. Unfortunately we don't have issue number information when we emit ambiguity error, and putting issue number into #[unstable_feature_bound]
can be quite verbose because there can be multiple symbol at a time, for example: #[unstable_feature_bound(foo, bar)]
So I added feature_err_unstable_feature_bound
in cf4770d, which is basically feature_err_issue
but omitted the issue number note.
tests/ui/unstable-feature_bound/unstable-feature-cross-crate-multiple-symbol.rs
Show resolved
Hide resolved
tests/ui/unstable-feature_bound/unstable_impl_coherence_inherence.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few more nits
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
#[stable(feature = "a", since = "1.1.1")] | ||
#[unstable_feature_bound(feat_bar)] | ||
fn bar() { | ||
Bar::foo(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not error in current implementation, we probably should lint against this usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as in, have an #[unstable_feature_bound(feat_bar)]
on a stable item?
yeah. Please add a fixme for this somewhere
if self.may_use_unstable_feature(param_env, symbol) { | ||
return self.evaluate_added_goals_and_make_canonical_response(Certainty::Yes); | ||
} else { | ||
return self.evaluate_added_goals_and_make_canonical_response(Certainty::Maybe( | ||
MaybeCause::Ambiguity, | ||
)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if self.may_use_unstable_feature(param_env, symbol) { | |
return self.evaluate_added_goals_and_make_canonical_response(Certainty::Yes); | |
} else { | |
return self.evaluate_added_goals_and_make_canonical_response(Certainty::Maybe( | |
MaybeCause::Ambiguity, | |
)); | |
} | |
if self.may_use_unstable_feature(param_env, symbol) { | |
self.evaluate_added_goals_and_make_canonical_response(Certainty::Yes) | |
} else { | |
self.evaluate_added_goals_and_make_canonical_response(Certainty::Maybe( | |
MaybeCause::Ambiguity, | |
)) | |
} |
@@ -20,6 +20,7 @@ rustc_parse_format = { path = "../rustc_parse_format" } | |||
rustc_session = { path = "../rustc_session" } | |||
rustc_span = { path = "../rustc_span" } | |||
rustc_transmute = { path = "../rustc_transmute", features = ["rustc"] } | |||
rustc_type_ir = {path = "../rustc_type_ir"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please go through rustc_middle
instead, it should reexport all of rustc_type_ir
in ty
#[allow(rustc::usage_of_type_ir_traits)] | ||
if self.selcx.infcx.may_use_unstable_feature(obligation.param_env, symbol) { | ||
return ProcessResult::Changed(Default::default()); | ||
} else { | ||
return ProcessResult::Unchanged; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[allow(rustc::usage_of_type_ir_traits)] | |
if self.selcx.infcx.may_use_unstable_feature(obligation.param_env, symbol) { | |
return ProcessResult::Changed(Default::default()); | |
} else { | |
return ProcessResult::Unchanged; | |
} | |
if self.selcx.infcx.may_use_unstable_feature(obligation.param_env, symbol) { | |
ProcessResult::Changed(Default::default()); | |
} else { | |
ProcessResult::Unchanged; | |
} |
cc @compiler-errors I don't think we should use the InferCtxtLike
trait here. However, I can't think of a good place to put this.
We could make it a free function and use the reexport from rustc_middle
, but 🤷 idk, also feels kinda iffy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, I don't see why it shouldn't just be a free function. Why does it use InferCtxtLike
today?
return Ok(EvaluatedToOk); | ||
} else { | ||
return Ok(EvaluatedToAmbig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return Ok(EvaluatedToOk); | |
} else { | |
return Ok(EvaluatedToAmbig); | |
Ok(EvaluatedToOk) | |
} else { | |
Ok(EvaluatedToAmbig) |
#[stable(feature = "a", since = "1.1.1")] | ||
#[unstable_feature_bound(feat_bar)] | ||
fn bar() { | ||
Bar::foo(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as in, have an #[unstable_feature_bound(feat_bar)]
on a stable item?
yeah. Please add a fixme for this somewhere
@@ -0,0 +1,15 @@ | |||
//@ aux-build:unstable_impl_coherence_inference_aux.rs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file is called tests/ui/unstable-feature_bound/unstable_impl_coherence_inherence.rs
rn, not inference
. and why call it coherence
🤔 this is more about method selection, is it not?
Could you please do a perf run before this lands too? The extra call to |
This PR allows marking impls of stable trait with stable type as unstable.
Approach
In std/core, an impl can be marked as unstable by annotating it with
#[unstable_feature_bound(feat_name)]
. This will add aClauseKind::Unstable_Feature(feat_name)
to the list of predicates inpredicates_of
.When an unstable impl's function is called, we will first iterate through all the goals in
param_env
to check if there is anyClauseKind::UnstableFeature(feat_name)
inparam_env
.The existence of
ClauseKind::Unstable_Feature(feat_name)
inparam_env
means an#[unstable_feature_bound(feat_name)]
is present at the call site of the function, so we allow the check to succeed in this case.If
ClauseKind::UnstableFeature(feat_name)
does not exist inparam_env
, we will still allow the check to succeed for either of the cases below:#[feature(feat_name)]
outside of std / core.For the rest of the case, it will fail with ambiguity.
Limitation
In this PR, we do not support:
#[unstable_feature_bound]
within stable APIs#[unstable_feature_bound]
#[unstable_feature_bound]
on items other than free function and implAcknowledgement
The design and mentoring are done by @BoxyUwU